This repository has been archived by the owner on Jan 25, 2023. It is now read-only.
DynamoDB storage backend #190
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brikis98
reviewed
Mar 24, 2020
| * `--s3-bucket` (optional): Specifies the S3 bucket to use to store Vault data. Only used if `--enable-s3-backend` is set. | ||
| * `--s3-bucket-path` (optional): Specifies the S3 bucket path to use to store Vault data. Default is `""`. Only used if `--enable-s3-backend` is set. | ||
| * `--s3-bucket-region` (optional): Specifies the AWS region where `--s3-bucket` lives. Only used if `--enable-s3-backend` is set. | ||
| * `--storage-backend` (optional): If this flag is set, the backend will be enabled in addition to the HA backend. Default is `consul`. |
There was a problem hiding this comment.
NIT: Should list all valid values (consul, s3, dynamo) for this param and the ones below.
| echo -e " --enable-dynamo-backend\tIf this flag is set, DynamoDB will be enabled as the backend storage (HA)" | ||
| echo -e " --dynamo-region\tSpecifies the AWS region where --dynamo-table lives. Only used if '--enable-dynamo-backend is on'" | ||
| echo -e " --dynamo--table\tSpecifies the DynamoDB table to use for HA Storage. Only used if '--enable-dynamo-backend is on'" | ||
| echo -e " --storage-backend\tStorage backend type to use for secrets. Default is consul" |
There was a problem hiding this comment.
NIT: Same here. Should list all supported values for the backend params.
| if [[ "$enable_s3_backend" == "true" ]]; then | ||
| s3_config=$(cat <<EOF | ||
| local vault_ha_storage_backend="" | ||
| if [[ "$storage_backend" == "consul" ]]; then |
There was a problem hiding this comment.
NIT: to keep this function from growing bigger and bigger, we should extract the code that sets the storage backend into a separate function that uses a case statement and returns the proper value by writing it to stdout. Same for HA storage.
| @@ -443,13 +468,15 @@ function run { | |||
| local systemd_stderr="" | |||
| local user="" | |||
| local skip_vault_config="false" | |||
| local enable_s3_backend="false" | |||
| local storage_backend="consul" | |||
| local ha_storage_backend="consul" | |||
There was a problem hiding this comment.
NIT: Store these values in readonly variables at the top. Use the variables here and in the docs.
|
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Change
run-vaultto support dynamo backend as either storage or HA storage.Why
The implementation of DynamoDB has limited support only allowing DynamoDB as both the
storageandha_storagebackend.It is valid to use DynamoDB as either. These combinations should all be valid but not currently possible
DynamoDBConsulDynamoDBThis change enables you to specify the
--storage-backendas any ofs3|dynamodb|consuland the--ha-storage-backendasdynamodb|consul.Consul is the default for both to maintain expected behavior